Conversation
Lifts the per-panel status indicator up to each collapsed accordion header so admins can see at a glance which providers are connected without expanding every section. Connected providers sort first. - Add optional status: and meta: locals to settings/_section partial; pill hides via group-open:hidden when the section is expanded - New settings/providers/_status_pill partial (ok/warn/err/off states) - Add SettingsHelper#provider_summary to centralise the connected-vs-not logic already scattered across panel partials - Refactor show.html.erb to pass status to every section and sort family_panels by connection state - Add settings.providers.status.* i18n keys - Add system tests asserting pill renders and sort order https://claude.ai/code/session_01KW2HCN9rP1fiyQuw7Cju9D
Partition the provider list in the controller into @connected_providers and @available_providers based on provider_summary status, and render each group under its own heading with a count. Auto-open the section when only one provider is connected. Adds an empty-state line when nothing is connected yet. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… error surfacing - Extend provider_summary to return :err/:warn with meta text by checking latest sync per item (window function, same pattern as ProviderConnectionStatus) and Enable Banking session expiry within 7 days - Partition provider entries into three groups: Connected (:ok), Action needed (:warn/:err, auto-opened), Available (:off) - Add Settings::HealthSummary ViewComponent — four-tile grid showing Connected, Action needed, Errors, and Accounts synced counts - Render health strip directly under page description; omit Action needed heading when group is empty - Add i18n keys for tile labels, group heading, and all meta strings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ect drawer - Add Provider::Metadata registry with static display data (region, kind, tier, maturity, logo) for all 11 providers - Add Settings::ProviderCard ViewComponent rendering logo square, name, Beta/Alpha pill, meta line (region · type · tier), tagline, and Connect link - Add connect_form action + route (GET /settings/providers/:key/connect_form) that opens the existing panel partial or config form in a DS::Dialog drawer - Replace the Available accordion loop with a 2-column responsive card grid; empty state when all providers are connected - Fix layout override: use turbo_rails/frame layout for frame requests so the drawer response is not wrapped in the full settings layout (was causing Turbo to pick the empty outer drawer frame instead of the filled one) - Add SyncAllProvidersJob and last_sync_all_attempted_at migration (sync-all throttle support) - Unify Connected + Action needed into a single "Your connections" section; items with warn/err status auto-open - Fix Enable Banking grouping: items with expired sessions were returning :off (Available) instead of :warn (Your connections); gate now checks any? instead of any?(&:session_valid?) - Add reconsent_required locale key for fully-expired EB sessions - Surface Beta/Alpha maturity pills on connected provider accordion rows via new badge: param on settings_section helper - Add i18n taglines for all 11 providers; add connect and empty_available keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Delete Settings::BankSyncController and its views (the providers page is
now a strict superset of what bank_sync offered)
- Add permanent 301 redirect: GET /settings/bank_sync → /settings/providers
- Collapse nav to a single "Bank Sync" entry pointing at /settings/providers;
remove the duplicate admin-only "Providers" entry from the Advanced section
- Remove "Providers" from SETTINGS_ORDER; point "Bank Sync" at
settings_providers_path for next/prev navigation
- Rename page title to "Bank Sync"; replace admin-credential lede with
user-facing copy ("Connect external accounts…")
- Update breadcrumb: Home → Bank sync
- Add controller test asserting 301 status and Location header
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR consolidates bank-sync into a unified provider-management workflow. It introduces a metadata registry for providers, refactors the ProvidersController with sync/connect actions and family panel orchestration, adds a SyncAllProvidersJob for bulk sync, unifies provider panel UX via shared setup-steps and alert components, adds JavaScript filtering, redirects legacy bank-sync routes, and expands test and localization coverage across six languages. ChangesProvider Management UI Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b59dd64c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| items: [ | ||
| { label: t(".accounts_label"), path: accounts_path, icon: "layers" }, | ||
| { label: t(".bank_sync_label"), path: settings_bank_sync_path, icon: "banknote" }, | ||
| { label: t(".bank_sync_label"), path: settings_providers_path, icon: "banknote" }, |
There was a problem hiding this comment.
Hide Bank Sync link from non-admin users
This link is rendered in the general settings nav for every user, but Settings::ProvidersController#show is still protected by ensure_admin, which redirects unauthorized users back to settings_providers_path. For family members, clicking the new Bank Sync nav item therefore lands on an admin-only route that redirects to itself rather than showing the old informational page or a usable destination.
Useful? React with 👍 / 👎.
|
|
||
| def perform(family_id) | ||
| family = Family.find(family_id) | ||
| family.sync_later |
There was a problem hiding this comment.
Schedule Binance in provider-wide syncs
For families with Binance connections, the new Sync all flow enqueues this job but then delegates to family.sync_later; Family::Syncer::SYNCABLE_ITEM_ASSOCIATIONS does not include :binance_items, even though this page now treats Binance as a connected/syncable provider via PANEL_SYNCABLE_TYPES. In that context the UI says all connected providers are syncing, but Binance is never scheduled unless the user clicks its per-row sync button.
Useful? React with 👍 / 👎.
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
db/schema.rb (2)
1390-1413:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDuplicate
account_number_maskcolumn insophtron_accountsis the direct cause of the CI failure.
account_number_maskis defined at line 1406 (pre-existing) and again at line 1409 (marked as changed). Railsdb:schema:loadraisesArgumentError: you can't define an already defined column 'account_number_mask' on 'sophtron_accounts'— confirmed by the pipeline failure.🐛 Proposed fix — remove the duplicate line
t.string "customer_id" t.string "member_id" t.string "account_number_mask" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "account_number_mask" t.index ["account_id"], name: "index_sophtron_accounts_on_account_id"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/schema.rb` around lines 1390 - 1413, The schema defines the column account_number_mask twice on the sophtron_accounts table which causes Rails to raise an ArgumentError during db:schema:load; remove the duplicate definition so account_number_mask is only declared once in the create_table block for sophtron_accounts (update the create_table "sophtron_accounts" block to delete the redundant t.string "account_number_mask" entry), then run schema lint/tests to confirm the CI error is resolved.
1432-1452:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
sophtron_itemshas 8 columns duplicated — will cause a seconddb:schema:loadfailure aftersophtron_accountsis fixed.
customer_id,customer_name,raw_customer_payload,user_institution_id,current_job_id,job_status,raw_job_payload, andlast_connection_errorall appear at lines 1432–1439 (existing) and again at lines 1442–1449 (marked as new). Additionally,last_connection_errorsilently changes type fromtexttostringbetween the two occurrences.The only genuinely new additions to this table should be
manual_sync(line 1450) andcurrent_job_sophtron_account_id(line 1451) plus the index (line 1452). The entire block at lines 1442–1449 should be removed.🐛 Proposed fix — remove the duplicated lines 1442–1449
t.text "last_connection_error" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "customer_id" - t.string "customer_name" - t.jsonb "raw_customer_payload" - t.string "user_institution_id" - t.string "current_job_id" - t.string "job_status" - t.jsonb "raw_job_payload" - t.string "last_connection_error" t.boolean "manual_sync", default: false, null: false t.uuid "current_job_sophtron_account_id" t.index ["current_job_sophtron_account_id"], ...If the intent was also to change
last_connection_errorfromtexttostring, that requires a separate migration withchange_column.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/schema.rb` around lines 1432 - 1452, The schema contains duplicated column definitions for the sophtron_items table; remove the repeated block that re-declares customer_id, customer_name, raw_customer_payload, user_institution_id, current_job_id, job_status, raw_job_payload, and last_connection_error so only the original column definitions remain and only add the new columns manual_sync and current_job_sophtron_account_id plus its index; ensure last_connection_error stays as text in the schema (any type change must be implemented via a separate migration using change_column rather than editing schema.rb).app/controllers/settings/providers_controller.rb (1)
143-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid redirecting unauthorized users back into the same protected action.
On
GET /settings/providers, a non-admin will hitensure_admin, get redirected tosettings_providers_path, and immediately hitensure_adminagain. That creates a redirect loop instead of denying access.Suggested fix
def ensure_admin - redirect_to settings_providers_path, alert: "Not authorized" unless Current.user.admin? + return if Current.user.admin? + + redirect_to root_path, alert: t("settings.providers.not_authorized") end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/settings/providers_controller.rb` around lines 143 - 145, The ensure_admin before_action is redirecting non-admins back to settings_providers_path, causing a redirect loop; update ensure_admin (method ensure_admin in providers_controller) to either render a 403/forbidden response (render plain: "Not authorized", status: :forbidden) or redirect to a safe non-protected route (e.g., root_path or dashboard_path) with the alert, instead of redirecting to settings_providers_path, so unauthorized users are not sent back into the same protected action.
🧹 Nitpick comments (5)
app/views/settings/providers/_status_pill.html.erb (1)
2-8: ⚡ Quick winCase statement computing CSS classes belongs in a helper or ViewComponent, not the template.
Per guidelines: "Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file." This partial is also reused across multiple views and has variants by status—criteria that specifically recommend a ViewComponent.
♻️ Suggested refactor — extract to a helper
# app/helpers/settings_helper.rb (or providers_helper.rb) def status_pill_classes(status) case status.to_sym when :ok then { dot: "bg-success", pill: "bg-success/10 text-success" } when :warn then { dot: "bg-warning", pill: "bg-warning/10 text-warning" } when :err then { dot: "bg-destructive", pill: "bg-destructive/10 text-destructive" } else { dot: "bg-gray-400", pill: "bg-gray-100 text-secondary" } end end<%# locals: (status:) %> -<% - dot_class, pill_class = case status.to_sym - when :ok then [ "bg-success", "bg-success/10 text-success" ] - when :warn then [ "bg-warning", "bg-warning/10 text-warning" ] - when :err then [ "bg-destructive", "bg-destructive/10 text-destructive" ] - else [ "bg-gray-400", "bg-gray-100 text-secondary" ] - end -%> +<% classes = status_pill_classes(status) %> +<% dot_class, pill_class = classes[:dot], classes[:pill] %>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_status_pill.html.erb` around lines 2 - 8, The template currently contains presentation logic (the case statement building CSS classes) which should be moved to a helper or ViewComponent; extract that logic into a new helper method (e.g., status_pill_classes(status)) or a StatusPillComponent that returns the dot and pill class values (as keys like :dot and :pill), then update the partial to call status_pill_classes(status) and use the returned classes instead of the inline case; ensure the helper/component handles symbolizing the status and preserves the same mappings for :ok, :warn, :err and the default case.app/jobs/sync_all_providers_job.rb (1)
6-6: ⚡ Quick winUse
find_bywith a nil guard to avoid unrecoverable job failures when a family is deleted mid-queue.
Family.findraisesActiveRecord::RecordNotFoundif the record is gone by the time the job runs. With Sidekiq's default retry behavior this leads to repeated failures until retries are exhausted.♻️ Proposed fix
- family = Family.find(family_id) - family.sync_later + family = Family.find_by(id: family_id) + family&.sync_later🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/jobs/sync_all_providers_job.rb` at line 6, Replace the unsafe Family.find lookup with a nil-safe lookup and early return: change the Family.find(family_id) call to Family.find_by(id: family_id) and add a guard like return unless family (or log and return) in the job's perform method (the place where family = Family.find(...) is used) so the job exits gracefully if the family was deleted instead of raising ActiveRecord::RecordNotFound and retrying.app/views/settings/_section.html.erb (1)
19-27: ⚡ Quick winGeneric section partial couples to a provider-specific partial.
_section.html.erbis a shared layout component, but line 24 hard-codes a dependency on"settings/providers/status_pill". If any other settings section ever needs astatus:value (e.g., a future "Accounts" health section), it would either not render correctly or render an unrelated provider pill. Theactionslocal already demonstrates the right pattern — pass pre-rendered content in from the caller.♻️ Suggested approach
Pass the rendered pill from the call site (e.g.
settings_section(status: render("settings/providers/status_pill", status: ...))), and drop the inlinerenderfrom the generic partial:- <%= render "settings/providers/status_pill", status: status %> + <%= status if status.present? %>The caller can then pass
status: capture { render "settings/providers/status_pill", status: :ok }or any other rendered content, keeping_section.html.erbsubsystem-agnostic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/_section.html.erb` around lines 19 - 27, The shared partial _section.html.erb currently hard-codes render "settings/providers/status_pill" for the status display, coupling this generic layout to a provider-specific partial; change it to accept rendered status content from the caller (like actions already does) by removing the inline render and using the passed-in status local as-is, and update callers to pass status: render("settings/providers/status_pill", status: ...) or status: capture { render "settings/providers/status_pill", status: :ok } so _section.html.erb remains subsystem-agnostic and callers control the actual markup.app/views/settings/providers/show.html.erb (1)
1-18: ⚡ Quick winMove the new page title and intro copy into i18n.
"Bank Sync"and the lede are hard-coded English while the surrounding UI already uses translations. Please switch these tot(...)so this page stays consistent with the rest of settings copy.As per coding guidelines, "Always use
t()helper for user-facing strings. Updateconfig/locales/en.ymlfor new strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/show.html.erb` around lines 1 - 18, Replace the hard-coded page title and intro copy in the view: change content_for :page_title, "Bank Sync" to use t("settings.providers.bank_sync.page_title") and replace the paragraph text ("Connect external accounts so transactions, balances and holdings flow into Sure automatically.") with t("settings.providers.bank_sync.lede"); then add corresponding entries under settings.providers.bank_sync in config/locales/en.yml (page_title and lede) with the existing English strings as values so the view uses the t(...) helper and stays consistent with other translations.app/components/settings/provider_card.rb (1)
2-18: ⚡ Quick winLocalize the maturity badge labels instead of hard-coding English here.
These strings are rendered in the UI, so storing
"Beta"/"Alpha"in the component bypasses locale switching. Keep translation keys (or symbols) in the constant and resolve them witht(...)at render time.As per coding guidelines, "Always use
t()helper for user-facing strings. Updateconfig/locales/en.ymlfor new strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/settings/provider_card.rb` around lines 2 - 18, Replace hard-coded English labels in MATURITY_LABELS with translation keys (e.g., :beta and :alpha or symbol strings) and resolve them via the Rails i18n helper in the maturity_label method; specifically, keep MATURITY_LABELS as a mapping of maturity symbols to translation keys, then update the maturity_label method to call t(...) on the mapped key (using the t helper), and add the corresponding entries under config/locales/en.yml so the UI strings are localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/settings/health_summary.html.erb`:
- Line 1: The grid currently forces four columns on all viewports (the <div
class="grid grid-cols-4 gap-2">) causing very narrow tiles on mobile; change the
grid classes to be responsive so small screens use two columns (or one) and
larger screens use four — e.g. replace or augment grid-cols-4 with responsive
Tailwind classes like grid-cols-2 sm:grid-cols-4 (or grid-cols-1 md:grid-cols-4)
on that same div to ensure tiles have a mobile-friendly layout.
In `@app/components/settings/provider_card.html.erb`:
- Line 4: Replace the raw Tailwind utility "text-white" with the design-system
functional token "text-inverse" in the span that renders the logo text (the span
showing <%= logo_text %>), i.e., update the class attribute on that span to
remove "text-white" and add "text-inverse" so it follows the functional token
convention used by the design system.
In `@app/controllers/settings/providers_controller.rb`:
- Around line 80-89: The sync_all action is race-prone because the
present?/>update pair can be run concurrently; make it atomic by performing a
single conditional DB update on the Family row and only enqueueing
SyncAllProvidersJob when that update succeeds. Replace the current
check+family.update!(last_sync_all_attempted_at: ...) with a conditional update
scoped to Current.family (e.g. a where on Family.id and a predicate that
last_sync_all_attempted_at is nil or <= 30.seconds.ago) and proceed to call
SyncAllProvidersJob.perform_later(family.id) only if the conditional update
affected one row; otherwise redirect with the "sync_all_recently" notice. Ensure
you reference sync_all, Current.family, last_sync_all_attempted_at, and
SyncAllProvidersJob in the change.
In `@app/views/settings/providers/_group_heading.html.erb`:
- Line 2: The div currently always emits id="" when the local variable anchor is
nil; change the ERB so the id attribute is only rendered when anchor is present
(e.g., use a conditional like `if anchor.present?` or an inline ERB conditional)
so the <div> element does not produce an empty id and is only anchorable when a
non-empty anchor value exists; update the line that generates the div (the one
referencing anchor) to conditionally include the id attribute rather than always
outputting id.
In `@app/views/settings/providers/_sync_button.html.erb`:
- Around line 3-9: The button created by button_to
(sync_provider_settings_providers_path) is icon-only and needs an explicit
accessible name; add an aria-label (or include hidden sr-only text) that matches
the sync action instead of relying on title. Update the button_to call to
include aria-label: recently_synced ? t("settings.providers.recently_synced") :
t("settings.providers.sync_provider") (or render an adjacent sr-only <span> with
the same translated text) so screen readers receive a clear label while keeping
the existing title, disabled, provider_key, and form attributes intact.
In `@db/migrate/20260508120000_add_last_sync_all_attempted_at_to_families.rb`:
- Line 1: The migration class AddLastSyncAllAttemptedAtToFamilies currently
inherits from ActiveRecord::Migration[8.0]; change its superclass to
ActiveRecord::Migration[7.2] so it matches the project's schema version and
coding guidelines (replace [8.0] with [7.2] in the class declaration). Ensure no
other parts of this file reference migration version 8.0.
In `@test/system/settings/providers_test.rb`:
- Around line 162-173: The test "clicking a provider card connect link opens the
connect drawer" is clicking the first Connect link in the grid
(find("a[data-turbo-frame='drawer']", match: :first)) which can target the wrong
provider; instead scope the click to the SimpleFIN card by locating the provider
card element that contains the "SimpleFIN" text (e.g., within the card element
matching its title/label) and then call find("a[data-turbo-frame='drawer']",
text: /Connect/).click inside that scoped block so the test always opens
SimpleFIN's drawer before asserting dialog[open] and "Setup Token".
- Around line 20-38: The tests "shows not configured pill for an unconfigured
provider" and "connected providers render before unconfigured ones" still query
accordion markup; update them to target the new available-provider card layout:
replace within("details", text: "SimpleFIN") with a within(...) that scopes to
the available-provider card container (e.g. the container/class that holds
`@available` provider cards) and assert the "Not configured" pill inside that card
for SimpleFIN; similarly change the second test to collect the visible provider
card headers (replace all("details summary") with all(...) selecting the
provider card header/title elements) and compare the index of the SimpleFIN card
vs the Binance card to assert ordering. Also apply the same selector changes to
the related assertions around lines 52-70 (other tests expecting
accordion/summary markup).
---
Outside diff comments:
In `@app/controllers/settings/providers_controller.rb`:
- Around line 143-145: The ensure_admin before_action is redirecting non-admins
back to settings_providers_path, causing a redirect loop; update ensure_admin
(method ensure_admin in providers_controller) to either render a 403/forbidden
response (render plain: "Not authorized", status: :forbidden) or redirect to a
safe non-protected route (e.g., root_path or dashboard_path) with the alert,
instead of redirecting to settings_providers_path, so unauthorized users are not
sent back into the same protected action.
In `@db/schema.rb`:
- Around line 1390-1413: The schema defines the column account_number_mask twice
on the sophtron_accounts table which causes Rails to raise an ArgumentError
during db:schema:load; remove the duplicate definition so account_number_mask is
only declared once in the create_table block for sophtron_accounts (update the
create_table "sophtron_accounts" block to delete the redundant t.string
"account_number_mask" entry), then run schema lint/tests to confirm the CI error
is resolved.
- Around line 1432-1452: The schema contains duplicated column definitions for
the sophtron_items table; remove the repeated block that re-declares
customer_id, customer_name, raw_customer_payload, user_institution_id,
current_job_id, job_status, raw_job_payload, and last_connection_error so only
the original column definitions remain and only add the new columns manual_sync
and current_job_sophtron_account_id plus its index; ensure last_connection_error
stays as text in the schema (any type change must be implemented via a separate
migration using change_column rather than editing schema.rb).
---
Nitpick comments:
In `@app/components/settings/provider_card.rb`:
- Around line 2-18: Replace hard-coded English labels in MATURITY_LABELS with
translation keys (e.g., :beta and :alpha or symbol strings) and resolve them via
the Rails i18n helper in the maturity_label method; specifically, keep
MATURITY_LABELS as a mapping of maturity symbols to translation keys, then
update the maturity_label method to call t(...) on the mapped key (using the t
helper), and add the corresponding entries under config/locales/en.yml so the UI
strings are localized.
In `@app/jobs/sync_all_providers_job.rb`:
- Line 6: Replace the unsafe Family.find lookup with a nil-safe lookup and early
return: change the Family.find(family_id) call to Family.find_by(id: family_id)
and add a guard like return unless family (or log and return) in the job's
perform method (the place where family = Family.find(...) is used) so the job
exits gracefully if the family was deleted instead of raising
ActiveRecord::RecordNotFound and retrying.
In `@app/views/settings/_section.html.erb`:
- Around line 19-27: The shared partial _section.html.erb currently hard-codes
render "settings/providers/status_pill" for the status display, coupling this
generic layout to a provider-specific partial; change it to accept rendered
status content from the caller (like actions already does) by removing the
inline render and using the passed-in status local as-is, and update callers to
pass status: render("settings/providers/status_pill", status: ...) or status:
capture { render "settings/providers/status_pill", status: :ok } so
_section.html.erb remains subsystem-agnostic and callers control the actual
markup.
In `@app/views/settings/providers/_status_pill.html.erb`:
- Around line 2-8: The template currently contains presentation logic (the case
statement building CSS classes) which should be moved to a helper or
ViewComponent; extract that logic into a new helper method (e.g.,
status_pill_classes(status)) or a StatusPillComponent that returns the dot and
pill class values (as keys like :dot and :pill), then update the partial to call
status_pill_classes(status) and use the returned classes instead of the inline
case; ensure the helper/component handles symbolizing the status and preserves
the same mappings for :ok, :warn, :err and the default case.
In `@app/views/settings/providers/show.html.erb`:
- Around line 1-18: Replace the hard-coded page title and intro copy in the
view: change content_for :page_title, "Bank Sync" to use
t("settings.providers.bank_sync.page_title") and replace the paragraph text
("Connect external accounts so transactions, balances and holdings flow into
Sure automatically.") with t("settings.providers.bank_sync.lede"); then add
corresponding entries under settings.providers.bank_sync in
config/locales/en.yml (page_title and lede) with the existing English strings as
values so the view uses the t(...) helper and stays consistent with other
translations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3452438-7448-40e0-b004-bbd46d7796fd
📒 Files selected for processing (26)
app/components/settings/health_summary.html.erbapp/components/settings/health_summary.rbapp/components/settings/provider_card.html.erbapp/components/settings/provider_card.rbapp/controllers/settings/bank_sync_controller.rbapp/controllers/settings/providers_controller.rbapp/helpers/settings_helper.rbapp/jobs/sync_all_providers_job.rbapp/models/provider/metadata.rbapp/views/settings/_section.html.erbapp/views/settings/_settings_nav.html.erbapp/views/settings/bank_sync/_provider_link.html.erbapp/views/settings/bank_sync/show.html.erbapp/views/settings/providers/_add_provider_cta.html.erbapp/views/settings/providers/_group_heading.html.erbapp/views/settings/providers/_status_pill.html.erbapp/views/settings/providers/_sync_button.html.erbapp/views/settings/providers/connect_form.html.erbapp/views/settings/providers/show.html.erbconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260508120000_add_last_sync_all_attempted_at_to_families.rbdb/schema.rbtest/controllers/settings/providers_controller_test.rbtest/system/settings/providers_test.rbtest/system/settings_test.rb
💤 Files with no reviewable changes (3)
- app/controllers/settings/bank_sync_controller.rb
- app/views/settings/bank_sync/_provider_link.html.erb
- app/views/settings/bank_sync/show.html.erb
|
Great consolidation — the unified provider page and health strip are a meaningful UX improvement. One design-system violation to fix before merge:
<span class="text-xs font-bold text-white"><%= logo_text %></span>
Replace with the appropriate functional token — likely (The Generated by Claude Code Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/models/family/syncer_test.rb (1)
12-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the
syncablescope in the Plaid expectation.
Family::Syncer#child_syncablesonly enqueuesfamily.plaid_items.syncable, but this test counts every Plaid item. If a non-syncable Plaid fixture is added, the assertion will fail for the wrong reason or stop guarding the real behavior.Minimal fix
- plaid_items_count = `@family.plaid_items.count` + plaid_items_count = `@family.plaid_items.syncable.count`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/family/syncer_test.rb` around lines 12 - 25, The test is counting all Plaid items but Family::Syncer#child_syncables only enqueues `@family.plaid_items.syncable`; change the setup and expectation to use the syncable scope so the assertion matches actual behavior—compute a variable (e.g., plaid_items_syncable_count = `@family.plaid_items.syncable.count`) and use that in the PlaidItem.any_instance.expects(...).times(...) expectation referencing Family::Syncer#child_syncables and `@family.plaid_items.syncable`.
♻️ Duplicate comments (1)
test/system/settings/providers_test.rb (1)
25-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the connect assertion to the SimpleFIN card.
This currently proves that the grid contains
SimpleFINsomewhere and that some card has aConnectlink. The test still passes if the SimpleFIN card loses its affordance and another provider supplies the link.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/system/settings/providers_test.rb` around lines 25 - 27, The test currently checks the grid and the presence of a Connect link separately; instead, scope the connect assertion to the SimpleFIN card by locating the provider card element that contains the text "SimpleFIN" (e.g. find or within on the element returned by available_provider_cards_container that has text "SimpleFIN") and then assert that within that card there is an "a[data-turbo-frame='drawer']" link with text "Connect"; update the assertions around available_provider_cards_container and the SimpleFIN text to perform the connect link check inside that card element rather than globally.
🧹 Nitpick comments (1)
test/system/settings/providers_test.rb (1)
39-45: ⚡ Quick winPrefer DOM-order assertions over
native.location.y.These checks depend on rendered pixel coordinates, which makes them much more brittle across driver and viewport changes than necessary. Asserting relative DOM order would cover the same behavior with a lot less flake risk.
Also applies to: 72-81, 137-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/system/settings/providers_test.rb` around lines 39 - 45, The test currently compares pixel positions via connections_heading.native.location.y, available_heading.native.location.y and page.find("details", text: "SimpleFIN").native.location.y which is brittle; instead assert DOM order—locate the same elements (connections_heading, available_heading, and the details element matching "SimpleFIN") and assert their relative order using a DOM-based approach (e.g. use Capybara XPath following/preceding relationships or compare indices from page.all to ensure the details element appears after connections_heading and before available_heading). Replace the three native.location.y comparisons with these DOM-order assertions in the same examples (also apply the same change to the other occurrences mentioned).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/settings/providers_controller.rb`:
- Line 4: The before_action currently gates :show behind ensure_admin
(before_action :ensure_admin, only: [ :show, :update, :sync_all, :sync,
:connect_form ]), which blocks non-admin family members from viewing the Bank
Sync page; remove :show from the only list so that ensure_admin continues to
protect write/sync actions (:update, :sync_all, :sync, :connect_form) but allows
non-admins to reach the show action.
- Around line 7-10: The controller currently hard-codes user-facing strings in
`@breadcrumbs` and in the flash messages (e.g., the breadcrumb entries created in
`@breadcrumbs` and the flash set around the block at lines ~69-75); replace those
literals with t(...) lookups (e.g., t('settings.providers.breadcrumbs.home'),
t('settings.providers.breadcrumbs.bank_sync') and
t('settings.providers.flash.success' / 'settings.providers.flash.error') used
where flash[:notice]/flash[:alert] are set) and ensure the capitalization is
"Bank Sync" in the locale value; then add the corresponding hierarchical keys
under the settings.providers namespace in the app locale file so all user-facing
text comes from I18n.
In `@app/views/settings/_section.html.erb`:
- Around line 19-26: The actions block is currently rendered inside the
<summary> (via the actions variable on the line that outputs <%= actions if
actions.present? %>), which nests interactive controls inside the disclosure
toggle; move the actions container out of the <summary> so it becomes a sibling
in the header row: render the same conditional (<%= actions if actions.present?
%> or the surrounding div that contains meta/status/actions) after the
</summary> while preserving the existing classes/structure (the div with "flex
items-center gap-2 shrink-0 group-open:hidden" and the meta/status logic) so the
actions remain visually in the header but are no longer inside the summary
element.
---
Outside diff comments:
In `@test/models/family/syncer_test.rb`:
- Around line 12-25: The test is counting all Plaid items but
Family::Syncer#child_syncables only enqueues `@family.plaid_items.syncable`;
change the setup and expectation to use the syncable scope so the assertion
matches actual behavior—compute a variable (e.g., plaid_items_syncable_count =
`@family.plaid_items.syncable.count`) and use that in the
PlaidItem.any_instance.expects(...).times(...) expectation referencing
Family::Syncer#child_syncables and `@family.plaid_items.syncable`.
---
Duplicate comments:
In `@test/system/settings/providers_test.rb`:
- Around line 25-27: The test currently checks the grid and the presence of a
Connect link separately; instead, scope the connect assertion to the SimpleFIN
card by locating the provider card element that contains the text "SimpleFIN"
(e.g. find or within on the element returned by
available_provider_cards_container that has text "SimpleFIN") and then assert
that within that card there is an "a[data-turbo-frame='drawer']" link with text
"Connect"; update the assertions around available_provider_cards_container and
the SimpleFIN text to perform the connect link check inside that card element
rather than globally.
---
Nitpick comments:
In `@test/system/settings/providers_test.rb`:
- Around line 39-45: The test currently compares pixel positions via
connections_heading.native.location.y, available_heading.native.location.y and
page.find("details", text: "SimpleFIN").native.location.y which is brittle;
instead assert DOM order—locate the same elements (connections_heading,
available_heading, and the details element matching "SimpleFIN") and assert
their relative order using a DOM-based approach (e.g. use Capybara XPath
following/preceding relationships or compare indices from page.all to ensure the
details element appears after connections_heading and before available_heading).
Replace the three native.location.y comparisons with these DOM-order assertions
in the same examples (also apply the same change to the other occurrences
mentioned).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b04ed6d2-1cd0-4554-8633-65478024a19c
📒 Files selected for processing (18)
app/components/settings/health_summary.html.erbapp/components/settings/provider_card.html.erbapp/components/settings/provider_card.rbapp/controllers/settings/providers_controller.rbapp/helpers/settings_helper.rbapp/jobs/sync_all_providers_job.rbapp/models/family/syncer.rbapp/views/settings/_section.html.erbapp/views/settings/_settings_nav.html.erbapp/views/settings/providers/_group_heading.html.erbapp/views/settings/providers/_status_pill.html.erbapp/views/settings/providers/_sync_button.html.erbapp/views/settings/providers/show.html.erbconfig/locales/views/settings/en.ymltest/controllers/settings/providers_controller_test.rbtest/models/family/syncer_test.rbtest/system/settings/providers_test.rbtest/system/settings_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/settings/en.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- app/components/settings/provider_card.rb
- app/components/settings/health_summary.html.erb
- test/system/settings_test.rb
- app/components/settings/provider_card.html.erb
- app/views/settings/providers/_group_heading.html.erb
- app/views/settings/providers/_sync_button.html.erb
- app/jobs/sync_all_providers_job.rb
- app/helpers/settings_helper.rb
- app/views/settings/_settings_nav.html.erb
- app/views/settings/providers/show.html.erb
| layout -> { turbo_frame_request? ? "turbo_rails/frame" : "settings" } | ||
|
|
||
| before_action :ensure_admin, only: [ :show, :update ] | ||
| before_action :ensure_admin, only: [ :show, :update, :sync_all, :sync, :connect_form ] |
There was a problem hiding this comment.
show is now admin-gated, so the shared Bank Sync entry dead-ends for family members.
Including :show here means a non-admin who follows the new Bank Sync nav item gets bounced to root_path with "Not authorized" instead of seeing the page. If the page is meant to be reachable for both admin and non-admin users per the PR objective, keep the write/sync actions restricted but leave show accessible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/controllers/settings/providers_controller.rb` at line 4, The
before_action currently gates :show behind ensure_admin (before_action
:ensure_admin, only: [ :show, :update, :sync_all, :sync, :connect_form ]), which
blocks non-admin family members from viewing the Bank Sync page; remove :show
from the only list so that ensure_admin continues to protect write/sync actions
(:update, :sync_all, :sync, :connect_form) but allows non-admins to reach the
show action.
| <% if status.present? %> | ||
| <div class="flex items-center gap-2 shrink-0 group-open:hidden"> | ||
| <% if meta.present? %> | ||
| <span class="text-xs text-subdued"><%= meta %></span> | ||
| <% end %> | ||
| <%= status %> | ||
| <%= actions if actions.present? %> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect where `actions:` is passed into the shared settings section
# and whether those callers render interactive controls.
rg -n -C3 'settings_section\(.*actions:|actions:' app/views app/helpers app/components
fd -a '_sync_button.html.erb' app/views app/components | xargs -r sed -n '1,160p'Repository: we-promise/sure
Length of output: 8656
🏁 Script executed:
cat -n app/views/settings/_section.html.erbRepository: we-promise/sure
Length of output: 2243
Move interactive actions outside the <summary> element.
The actions are currently rendered inside the <summary> tag (line 25), which nests interactive controls within the disclosure toggle. This violates semantic HTML structure. Even though the sync button uses event.stopPropagation(), it's not a reliable solution for all interaction patterns and leaves the semantic structure broken. Move the actions container to be a sibling of <summary>, rendered as part of the header row but outside the toggle element itself.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/views/settings/_section.html.erb` around lines 19 - 26, The actions block
is currently rendered inside the <summary> (via the actions variable on the line
that outputs <%= actions if actions.present? %>), which nests interactive
controls inside the disclosure toggle; move the actions container out of the
<summary> so it becomes a sibling in the header row: render the same conditional
(<%= actions if actions.present? %> or the surrounding div that contains
meta/status/actions) after the </summary> while preserving the existing
classes/structure (the div with "flex items-center gap-2 shrink-0
group-open:hidden" and the meta/status logic) so the actions remain visually in
the header but are no longer inside the summary element.
Picks up the remaining items from Claude Design's review of #1710 that the previous review-feedback commit didn't cover. DS / casing - Sentence-case the page title ("Bank Sync" -> "Bank sync") and align the nav label. - Drop the card hover-lift (shadow-border-sm) in favour of bg-container-hover; per the DS, card hover is colour-only. - Whole-tile click target on each provider card — the inner "Connect ->" link was a hit-target inversion. - Set Sync all to whitespace-nowrap so the label stops wrapping at narrow viewport widths. UX simplifications - Drop the four health-summary tiles (per-row warn/err pills already surface the signal at the scale this app sees). Removes Settings::HealthSummary, the @health_counts controller block, and the now-unused health.* locale keys. - Hide "Your connections" heading + empty-state line when no providers are connected — the lede already invites a connect. - Drop the redundant "Free" tier from per-card meta lines (printed 10x for one fact); "Paid" still surfaces on Plaid. Tests updated to drop the obsolete tiles assertion and switch the provider-card click selector to look up the (now whole-card) anchor by provider name.
…rch + kind filter Per the design review, the "Add another provider · Browse providers" card was a redirect to content one scroll-tick away. A search input plus kind chips lets users self-segment the catalog and is the right tool once it grows beyond the four to twelve providers we ship today. - New providers_filter Stimulus controller — case-insensitive free text search across name/region/kind, plus a chip group with All / Banks / Crypto / Investment that toggle visibility via Tailwind's `hidden` class. - _search_filters partial: search box (count-pluralized placeholder) + chip group, ARIA-labelled and aria-pressed for the chips. - ProviderCard exposes filter_data (target + name/region/kind data attrs) so the controller can match without re-rendering. - Lunchflow's `kind` was "Lunch" — switched to "Bank" so it falls under the Banks chip alongside its actual offering (it aggregates banks). - Drops the add_provider_cta partial and its locale entries; adds search_filters.* and an empty_filter message.
Two consistency wins from the screenshot/DS audit pass. Sync icon button now renders DS::Button (variant: icon, size: sm) instead of a hand-rolled `button_to`. Same component used by other icon-only actions across the app (settings/profiles, layouts/imports). Visual delta: 28×28 → 32×32 (DS sm size). Accept the +4px for consistency. `event.stopPropagation()` still wired via the form opt so the row's <details> doesn't toggle when the user clicks the button. Group heading now follows the established Sure section-label style (`text-xs font-medium text-secondary uppercase`) used by `_settings_nav` and the imports/categories surfaces. The previous sentence-case `text-sm text-primary` was a one-off that didn't match the rest of the app. Locale strings stay sentence-case; uppercase comes from CSS `text-transform`. Tests updated to case-insensitively match the rendered heading text.
Follow-up tracking — design-system & UX work intentionally deferredUpdated as of branch head DS amendments (one or more separate PRs against
|
| Primitive | Why | Replaces |
|---|---|---|
DS::Tag (or DS::Badge) |
The new _maturity_badge + _status_pill and 4–10 hand-rolled pill instances across transactions/, settings/profiles/, budget_categories/ all express the same idea (tiny rounded-full chip with a label, optional dot/icon, semantic colour). |
_maturity_badge.html.erb, _status_pill.html.erb, transactions/_transfer_match.html.erb "Auto-matched", profiles/show.html.erb "ADMIN/MEMBER", budget_categories/_budget_category.html.erb "shared", and 4+ inline pills in transactions/_transaction.html.erb. |
DS::SearchInput |
Same icon-overlay search input shape (relative wrapper + absolute leading icon + border border-secondary rounded-md py-2.5 pl-10 pr-3 bg-container focus:ring-gray-500) lives in 6 places already. |
transactions/searches/filters/_{tag,category,merchant,account}_filter.html.erb, accounts/show/_activity.html.erb, splits/_category_select.html.erb, plus the new settings/providers/_search_filters.html.erb. |
DS::SegmentedControl (or extend DS::Tabs) |
Non-routing toggle chip group. Currently re-implemented as bg-surface-inset rounded-xl p-1 ... with aria-pressed chips. |
Inline chip groups; the existing DS::Tabs(variant: :default) is coupled to URL routing and panel switching, so it isn't quite the right shape. |
DS extensions (richer slot APIs)
DS::Dialog#headerleading slot — currentlywith_header(title:, custom_header:)block content lands BELOW the title row. Useful for the drawer header lock-up (logo chip + title + maturity badge inline) without bypassing the close button. Once landed,app/views/settings/providers/_drawer_header.html.erbcan be dropped.DS::Disclosurerich-summary slot — currently_connection_row.html.erbis hand-rolled<details>because DS::Disclosure's summary is text-only. Asummary_contentslot would let the row reuse the chevron rotation + open-state styling.
Provider-specific cleanup
Provider::Metadataraw colour palette —bg-blue-600,bg-orange-500, etc. are pinned per provider. Designer's deferred §01 BLOCKER. Two options: (a) lift to token namespace (bg-provider-coinbase); (b) drop colour entirely and use neutralbg-surface-inset. Needs a product call.- Tier inconsistency — only Plaid + Plaid EU have
tier:in metadata after this PR. Either drop thetier:field fromSettings::ProviderCardentirely or restore tier values for every provider.
UX follow-ups
- Filter state in URL params —
providers_filter_controller.jskeeps the active chip + search query in memory only. Convention 3 (CLAUDE.md) says query-params over in-memory state for snapshot-restore + back-nav resilience.
i18n backlog
The new keys ship in en.yml only and fall back to English for non-EN locales. Translations needed for:
settings.providers.bank_sync.page_title,bank_sync.ledesettings.providers.maturity.{beta,alpha}settings.providers.search_filters.aria_label,search_filters.placeholder,search_filters.chips.{all,bank,crypto,investment}settings.providers.empty_filtersettings.providers.drawer_trust_statementsettings.providers.taglines.plaid_eu
Across ca, de, es, fr, hu, nl, pl, pt-BR (8 locales).
Items addressed since the original comment
- ✅ Slim health strip removed entirely (per-row warn/err pills + status pills already carry the signal at the scale this app sees).
- ✅ Sync icon button migrated to
DS::Button(variant: :icon, size: :sm). - ✅ "Your connections" / "Available" headings now use the established Sure section-label style (
text-xs font-medium text-secondary uppercase). - ✅
plaid_eumetadata + tagline + display name added (no longer falls through to gray-500 default). - ✅ All arbitrary
*-[Npx]Tailwind values exceptmin-w-[200px]swapped to scale tokens. - ✅ Drawer trust statement border swapped from
border-secondary/10toborder-tertiary(the project's divider token).
Audit transcripts
The findings above are the synthesis of three independent agent reviews of this branch (CodeRabbit-style code review, Rails/Hotwire convention review against CLAUDE.md, screenshot paradigm comparison against /light/ reference shots, plus a Codex-style security review which came back clean).
`plaid_eu` is registered as a separate Provider::ConfigurationRegistry entry but had no Provider::Metadata row, so its card in the Available grid fell through to the gray-500 default and rendered empty (no region, kind, tier, or tagline). The title also came out as "Plaid Eu" because `titleize` doesn't know "EU" is an initialism. - Add a `plaid_eu` row to Provider::Metadata::REGISTRY with the same shape as `plaid` (US → EU, otherwise identical). - Introduce an optional `name:` field in metadata; controller falls back to it before titleizing the provider key. Lets `plaid_eu` render as "Plaid EU". - Add the missing `settings.providers.taglines.plaid_eu` translation.
`items-start` made the button hug the first line when the lede wrapped; on a single line the button sat at the top of the text bounding box which read slightly off. Center matches the dominant convention across the rest of settings (api_keys, securities, hostings, _section, _settings_nav_link_large).
… warnings Round of design-feedback fixes. Provider chips - Drop the per-provider raw Tailwind palette (bg-blue-600 etc.) from Provider::Metadata. All cards + drawer logo lock-up now use bg-surface-inset + text-primary, matching the design's §04 "drop colour entirely" recommendation. Solves the long-standing §01 BLOCKER without externalising brand assets. Re-introducing logos later just means an optional logo_svg: field on metadata. - ProviderCard component drops the `logo_bg:` parameter; the chip is now styled in the template. Filter / search - "Available · N" count and the empty-filter state now update client-side as the chip filter and free-text search narrow the grid (new `count` Stimulus target + dedicated update path). - Empty-filter state now offers a Clear filters button that resets both the search input and the active chip in one click. - Search placeholder drops the drifting "Search 9 providers" count for plain "Search providers" — the section heading carries the number. - Chip labels normalised to plural where natural: "Banks · Crypto · Investments" (Crypto stays as the mass noun). Drawer copy / treatment - "IP Whitelisting Required" → "IP whitelisting required" (DS sentence-case). - Binance "do NOT enable withdrawal permissions" lifted out of inline red-text into a proper bg-warning-50 border-warning-200 alert block with an alert-triangle icon. Matches the api_keys / hosting alert pattern. - SnapTrade free-tier inline alert-triangle now uses `size: "sm"` so the icon stops rendering at 20px next to 14px body text. Spacing - Group-heading margin top bumped 5 → 6 (20→24px) so the eyebrow has more breathing room above the search bar.
…in-card Two consistency fixes from a design-review pass. DS::Alert adoption - Replaces 9 hand-rolled error blocks across the provider panels (`bg-destructive/10 text-destructive ... line-clamp-3`) with `DS::Alert(variant: :error)` — the project's existing primitive. - Replaces the just-shipped Binance no-withdraw warning block with `DS::Alert(variant: :warning)` instead of a hand-rolled `bg-warning-50 border-warning-200` card. - Replaces the SnapTrade free-tier inline icon-prefixed warning paragraph with `DS::Alert(variant: :warning)` — proper alert treatment for an actual warning, not body copy. - Replaces the Enable Banking "Configuration locked" inline `bg-warning/10` two-paragraph block with `DS::Alert(variant: :warning)` using `safe_join` for the title + body. - Replaces the encryption-error block at the top of show.html.erb with `DS::Alert(variant: :error)`, again via `safe_join`. Mercury card-within-card - The "Add another Mercury connection" form was wrapped in a `<details>` `bg-container shadow-border-xs rounded-xl` card. In the Connect drawer (always 0 existing connections), that wrapping card-inside-the-drawer-card has no value — the form is the only thing on the surface. Drop the wrapper when no connections exist; keep the heading + form inline. When 1+ connections exist (the section page) the heading hints "+ Add another connection" without the disclosure indirection. Trade-off: the error-alert blocks lose their `line-clamp-3` / `title=` truncation. Acceptable for now — DS::Alert can grow a truncate option as a follow-up if needed. Open follow-up: DS::Alert itself uses raw Tailwind palette (`bg-yellow-50` etc.) instead of semantic tokens, and only accepts a single string `message:`. A separate issue tracks this.
DS::Alert convention across the rest of the app: alerts sit at the top of the form / page / section, not floating between content blocks. The Binance no-withdraw warning and SnapTrade free-tier warning were rendering between the setup-instructions list and the form fields — visually wonky. Move both to the top of their respective panels so the warning is the first thing the user sees when the connect drawer opens. Existing precedents this aligns with: - accounts/_form.html.erb (error alert above form) - valuations/new.html.erb (error alert above form) - other_assets/new.html.erb (info alert above form) - holdings/show.html.erb (warn alerts above content)
`items-start` on the container made the icon's top edge flush with the text's top edge, leaving the icon's optical center sitting below the text's first-line center. The hand-rolled alerts elsewhere in the codebase (api_keys/new, hostings/_sync_settings, holdings/show) all add `mt-0.5` to the icon for the same reason — fold that into the primitive so every caller gets the cap-height alignment.
Copy expert pass on the new provider drawer alerts. House style:
sentence case for titles, lead with the action, drop "Warning:" /
"Please" filler (the alert variant icon already signals tone),
prefer one short sentence + optional title-paragraph for emphasis.
- Binance no-withdraw warning: was a single line "Warning: do NOT
enable withdrawal permissions" — alarmist without context. Now
splits into "Read-only key only" (title) + "Don't enable
withdrawal permissions when creating your Binance API key — Sure
only needs read access." (body).
- SnapTrade free-tier note: "Free tier includes 5 brokerage
connections. Additional connections require a paid SnapTrade
plan." → "SnapTrade's free tier covers 5 brokerage connections.
Upgrade on SnapTrade for more."
- SnapTrade connection-limit-info inside the brokerage list: cut
entirely. The drawer already shows the cap; restating it in the
list was noise.
- SnapTrade needs-registration: "Credentials saved — finish
registration to connect a brokerage." → "Credentials saved.
Finish setup to connect a brokerage." ("registration" was
ambiguous — register where, with whom?)
- Enable Banking "Configuration locked" body: "Credentials cannot
be changed while you have active bank connections. Remove all
connections first to update credentials." → "Disconnect all
linked banks before changing these credentials." Same meaning,
half the words.
- Encryption-error block: title-cased "Encryption Configuration
Required" → "Encryption keys missing"; body strips "Please
ensure" filler and the parenthetical credential dump, leaving
the three credential names inline as a clean list. Self-hosters
still get exactly the names they need to set.
…uctions Per the design's drawer-cleanup follow-up. Replaces the per-panel "Setup instructions:" + ordered list + "Field descriptions:" block with a shared boxed-step component. The new partial — `_setup_steps.html.erb` — takes a `steps:` array of strings (or html_safe strings for inline links / code) plus an optional `help:` hash for a docs link below the steps. The eyebrow label is "Setup" (uppercase, tracking-wider) matching Sure's other section labels. Applied across all eleven provider panels: - _provider_form (Plaid + Plaid EU): field descriptions move to per-field helper text below the input. - _binance, _coinbase, _coinstats, _indexa_capital, _lunchflow, _mercury, _simplefin, _snaptrade, _sophtron, _enable_banking: ordered list + duplicate "Field descriptions" block both replaced by the partial. - Some panels' inline copy tightened in the same pass (Lunch Flow, SimpleFIN, Enable Banking) — the design copy is shorter than the current legacy strings; a copy-pass through every panel can follow as a separate cleanup. Token notes: uses scale tokens (`rounded-xl`, `text-xs`/`text-sm`, `tracking-wider`) instead of the design mock's exact arbitrary values, per the consistency-over-design-specs directive on this branch.
…otes
Read-flow audit on each connect drawer. The uniform `space-y-4`
treated every block (alert, steps, info card, fields, button) the
same — visually they were five sibling boxes with no grouping. The
fix is per panel; some notes belong as helper text on a specific
field, others as a tightly-grouped pre-fill primer.
Per panel:
- Binance: IP-whitelisting card now matches the setup_steps box
(`bg-surface-inset rounded-xl`) and is wrapped with setup_steps
in an inner `space-y-2` so they read as a single pre-fill primer
cluster. Same eyebrow treatment ("IP whitelisting required") so
the two boxes look like sister panels, not unrelated chrome.
- SnapTrade: drop the description paragraph above setup_steps. The
available-providers card grid already markets SnapTrade
("Connect brokerage accounts via the SnapTrade aggregation
network."); repeating in the drawer was duplication.
- Mercury: move the sandbox-API note out of its standalone <p>
below setup_steps and into per-field helper text under the
base_url field — the user only cares about the sandbox URL when
they're filling that field. Applied to both the per-item edit
form and the add-new form.
- _setup_steps partial: drop the now-pointless `mb-2` (outer
`space-y-4` already controls the gap; bottom-margin was dead
CSS thanks to margin-collapse rules with the next sibling's
margin-top).
Two unifying fixes after the panel-by-panel screenshots showed
mixed treatments.
Plaid + Plaid EU
- The registry-driven panel (_provider_form) was still rendering
each adapter's markdown `description` block as plain prose
("Setup instructions: 1. Visit the Plaid Dashboard ..."). Other
panels switched to the SetupSteps box; Plaid was the odd one out.
- Drop the markdown `description` block from both plaid_adapter
and plaid_eu_adapter. Render setup_steps in _provider_form for
these two provider keys via inline ERB (link helper handles the
Plaid Dashboard link cleanly; the regional differences fold to
the same dashboard URL with a different account scope).
- Other registry-based providers fall through to the previous
markdown description path — no behavior change for them.
Indexa Capital
- The API token field was wrapped in a `bg-surface border` "card"
that duplicated the field label inside as a heading and put the
description above the input. Same pattern the user flagged as
the "card within input" anti-shape.
- Drop the wrapper. The styled-form input renders its own label;
description moves to per-field helper text below the input,
matching the pattern used by Plaid (provider_form) and Mercury.
ScreenshotsCaptured at 1600×900 against the local app. Bank sync index, default viewConnected providers grouped at the top, search-led available grid below. Search filterFree-text search narrows the available grid in place; the count updates with each keystroke. Kind chip filterChips toggle the grid between All, Banks, Crypto, and Investments. Empty filter state + clear-filters affordanceWhen no provider matches the current search + chip combination, an empty-state row offers a one-click reset. Error state on a connection rowRed outline + Error pill + meta line ("Sync error") for any connection whose latest sync failed. Available card layoutLogo chip, title, region · kind · tier metadata, optional maturity badge (Alpha/Beta), and a tagline. Hover lifts the border. Connect drawerDrawer header lockup: logo chip + title + maturity badge + close affordance. SetupSteps partial provides numbered, localized setup instructions, followed by the credentials form and a trust statement at the bottom. Configured registry-driven provider surfaces in Your connectionsRegression guard for the Warn-state row + expanded drawerWarn outline ( Connected provider drawerConnected provider drawer reuses the same SetupSteps partial as the connect drawers, keeping setup guidance consistent across states. Connect drawer header lockupLogo chip + provider title + maturity badge inline at the top, close button right-aligned. Setup-steps card and form below. |
…text provider_summary had no plaid_eu branch — configured plaid_eu was falling through to status :off and rendering in Available even with credentials set. Collapse plaid + plaid_eu into a single registry check. Drawer title for non-panel configurations was provider_key.titleize, which produced "Plaid Eu" while the available card grid used metadata[:name] = "Plaid EU". Read from metadata first. While here: - compute_provider_sync_health no longer relies on instance_variable_get; pass family_panel_items explicitly so the hash-key/ivar-name coupling is gone. - drop unused .includes(:syncs, :mercury_accounts) and .includes(:snaptrade_accounts) from prepare_show_context. The show view only consults summary[:status]; the eager-loads were carried over from connect_form (which has its own load_provider_items).
…ults The plaid + plaid_eu setup steps in _provider_form.html.erb were hardcoded English strings. Move them to settings.providers.plaid_panel (shared) + plaid_eu_panel (EU-specific step 1) so they can be translated like every other panel. _setup_steps.html.erb was passing default: "Setup" / "Need help?" to t(), masking missing translations in non-EN locales. Both keys exist in en.yml — drop the defaults so missing translations actually surface.
Three system test additions: - Configured plaid_eu surfaces in Your connections (regression guard for the helper fix; previously fell through to Available). - Clear filters button resets input + chip state and brings cards back into view. - :warn-state connection row carries the border-warning/25 outline that distinguishes it from an :ok row.
Sweep through every string this branch added and replace em-dash splices with full sentences or simple connectives. en.yml: - drawer_trust_statement now reads "Read-only access. Sure can never move money, and your credentials are stored encrypted." instead of em-dash splicing. - sync_all_recently / recently_synced split into two sentences. - binance_panel.no_withdraw_body, plaid_panel.step_1_html / step_2, plaid_eu_panel.step_1_html same treatment. Hardcoded panel steps (enable_banking, lunchflow, simplefin) become "Go to <link> and …" or "Go to <link> for …" instead of the "<link> — get …" splice. Same setup_steps comment cleaned up.
b03a74d to
95eec78
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/views/settings/providers/_coinstats_panel.html.erb (1)
35-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace raw Tailwind color utility with design token.
The submit button uses
focus:ring-gray-900, which is a raw Tailwind color utility. Replace it with a design-system focus ring token to ensure consistent theming and dark-mode support. As per coding guidelines, functional tokens from the design system should be used instead of raw color utilities.Based on learnings from PR comments, raw color utilities like
text-whiteandbg-*-600were flagged for replacement with design-system tokens to ensure theme responsiveness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_coinstats_panel.html.erb` around lines 35 - 36, The submit button currently uses a raw Tailwind color utility `focus:ring-gray-900` in the `form.submit` call; replace that class with the design-system focus-ring token class used across the app (e.g. the project’s focus ring token such as `focus:ring-design-focus` or the equivalent token class your design system provides) so the `class:` string on the `form.submit` no longer contains `focus:ring-gray-900` but the appropriate token-based class for consistent theming and dark-mode support.app/views/settings/providers/_provider_form.html.erb (1)
27-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded user-facing copy bypasses I18n.
"Configuration can be set via environment variables or overridden below."(Line 30), the"Optional"placeholder fallback (Line 57), and"Save and connect"submit label (Line 66) are inline English. As per coding guidelines: "Always uset()helper for user-facing strings."Suggested fix
- <p class="text-sm text-secondary"> - Configuration can be set via environment variables or overridden below. - </p> + <p class="text-sm text-secondary"> + <%= t("settings.providers.provider_form.env_overridable") %> + </p> ... - placeholder: field.default || (field.required ? "" : "Optional"), + placeholder: field.default || (field.required ? "" : t("settings.providers.provider_form.optional")), ... - <%= form.submit "Save and connect", + <%= form.submit t("settings.providers.provider_form.save_and_connect"),As per coding guidelines: "Always use
t()helper for user-facing strings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_provider_form.html.erb` around lines 27 - 67, Replace hardcoded user-facing strings in this partial with i18n lookups: use t(...) instead of the literal "Configuration can be set via environment variables or overridden below." (the env_configured message), replace the placeholder fallback "Optional" used when computing placeholder for form.text_field (see field.default / field.required logic), and replace the submit label "Save and connect" passed to form.submit. Ensure you call the Rails t helper with appropriate keys (e.g. settings.providers.env_hint, settings.providers.optional_placeholder, settings.providers.save_and_connect) and update locale files accordingly so the view uses t(...) for these three strings.
🧹 Nitpick comments (11)
app/views/settings/providers/_coinstats_panel.html.erb (1)
4-6: ⚡ Quick winRemove redundant
.html_safecalls.Rails I18n automatically marks translation keys ending in
_htmlas HTML-safe. The explicit.html_safecalls on lines 4 and 6 are redundant and can be removed.♻️ Proposed refactor
<%= render "settings/providers/setup_steps", steps: [ - t("coinstats_items.new.step1_html").html_safe, + t("coinstats_items.new.step1_html"), t("coinstats_items.new.step2"), - t("coinstats_items.new.step3_html", accounts_url: accounts_path).html_safe + t("coinstats_items.new.step3_html", accounts_url: accounts_path) ] %>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_coinstats_panel.html.erb` around lines 4 - 6, Remove the redundant .html_safe calls on the translation helpers: replace occurrences of t("coinstats_items.new.step1_html").html_safe and t("coinstats_items.new.step3_html", accounts_url: accounts_path).html_safe with plain t("coinstats_items.new.step1_html") and t("coinstats_items.new.step3_html", accounts_url: accounts_path) respectively, since Rails marks `_html` I18n keys as HTML-safe automatically.app/views/settings/providers/_sophtron_panel.html.erb (1)
43-44: ⚡ Quick winConsider extracting button styling to a reusable component.
The submit button uses a verbose inline class string with 13 utility classes. While the color tokens (
text-inverse,bg-inverse,bg-inverse-hover,focus:ring-primary) correctly use design-system tokens, extracting this pattern into aDS::Buttoncomponent (if one exists) or helper would improve maintainability and ensure consistency across forms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_sophtron_panel.html.erb` around lines 43 - 44, The submit button in the form uses a long inline class string on the form.submit call; extract that styling into a reusable design-system button component or helper (e.g., DS::Button or a view helper like sophtron_button_class) and replace the inline class usage in the form.submit invocation (the call that uses is_new_record and t("sophtron_items.sophtron_panel.save"/"update")) so the markup passes a single component/helper and the label (derived from is_new_record and t(...)) while the component encapsulates the utility classes (text-inverse, bg-inverse, bg-inverse-hover, focus:ring-primary, etc.) and ensures consistent styling across forms.app/views/settings/providers/_simplefin_panel.html.erb (2)
2-7: ⚡ Quick winUse i18n keys instead of hardcoded English strings.
The setup steps contain hardcoded English text. Per coding guidelines, user-facing strings should use the
t()helper with hierarchical i18n keys. The PR objectives note that i18n is a known backlog item for 8 locales, but new English strings should still be added toconfig/locales/views/settings/en.ymlto facilitate future translation.🌐 Refactor to use i18n keys
Add keys to
config/locales/views/settings/en.yml:settings: providers: simplefin_panel: step_1_html: "Go to %{link} for a one-time setup token." step_2: "Paste the token below and connect." step_3: "Then head to Accounts to link your synced accounts." bridge_link_text: "SimpleFIN Bridge"Then update the template:
<%= render "settings/providers/setup_steps", steps: [ - ("Go to " + link_to("SimpleFIN Bridge", "https://beta-bridge.simplefin.org", target: "_blank", rel: "noopener noreferrer", class: "text-primary font-medium underline") + " for a one-time setup token.").html_safe, - "Paste the token below and connect.", - "Then head to Accounts to link your synced accounts." + t("settings.providers.simplefin_panel.step_1_html", link: link_to(t("settings.providers.simplefin_panel.bridge_link_text"), "https://beta-bridge.simplefin.org", target: "_blank", rel: "noopener noreferrer", class: "text-primary font-medium underline")).html_safe, + t("settings.providers.simplefin_panel.step_2"), + t("settings.providers.simplefin_panel.step_3") ] %>As per coding guidelines: "Always use
t()helper for user-facing strings. Organize i18n keys hierarchically by feature."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_simplefin_panel.html.erb` around lines 2 - 7, The template renders setup steps with hardcoded English strings; replace those literals with i18n lookups using the t() helper and hierarchical keys (e.g., settings.providers.simplefin_panel.step_1_html, step_2, step_3 and bridge_link_text) and update the call to render "settings/providers/setup_steps" to pass the translated strings (step_1_html should use t(..., link: link_to(...)) to keep the HTML link), and add the corresponding keys and English values to config/locales/views/settings/en.yml so translations are available.
26-26: ⚡ Quick winReplace raw Tailwind focus ring colors with design-system tokens.
The focus ring uses
focus:ring-gray-900andtheme-dark:focus:ring-white, which are raw Tailwind color utilities. Per coding guidelines, use functional design-system tokens instead (e.g.,focus:ring-primaryor a focus token defined insure-design-system.css).♻️ Replace with design-system tokens
- class: "inline-flex items-center justify-center rounded-lg px-4 py-2 text-sm font-medium text-inverse button-bg-primary hover:button-bg-primary-hover focus:outline-none focus:ring-2 focus:ring-gray-900 theme-dark:focus:ring-white focus:ring-offset-2 transition-colors" %> + class: "inline-flex items-center justify-center rounded-lg px-4 py-2 text-sm font-medium text-inverse button-bg-primary hover:button-bg-primary-hover focus:outline-none focus:ring-2 focus:ring-primary focus:ring-offset-2 transition-colors" %>As per coding guidelines: "Always use functional tokens defined in design system such as
text-primaryinstead oftext-white,bg-containerinstead ofbg-white,border border-primaryinstead ofborder border-gray-200."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_simplefin_panel.html.erb` at line 26, The focus ring classes in the button class string on the SimpleFin panel (the long class attribute in _simplefin_panel.html.erb) use raw Tailwind colors `focus:ring-gray-900` and `theme-dark:focus:ring-white`; replace those with the design-system focus tokens (for example `focus:ring-primary` or the appropriate focus token from sure-design-system.css) so the class string uses functional tokens instead of raw colors; update the class attribute where that button's classes are defined to remove `focus:ring-gray-900` and `theme-dark:focus:ring-white` and add the corresponding design-system token(s) for both light and dark theme focus states.app/views/settings/providers/_coinbase_panel.html.erb (2)
21-22: ⚡ Quick winReplace raw Tailwind color utilities with design-system tokens.
Per the PR comments from jjmata and coding guidelines, avoid raw Tailwind utilities:
- Line 21:
bg-[#0052FF]is a hardcoded hex color (Coinbase brand blue)- Line 22:
text-whiteshould use a functional design-system tokenIf
#0052FFis an intentional brand color, consider adding it to the design-system palette. Otherwise, use an existing surface token. For the icon text, usetext-inverseor another appropriate functional token so it responds to themes.♻️ Replace with design-system tokens
- <div class="w-10 h-10 rounded-full bg-[`#0052FF`] flex items-center justify-center"> - <%= icon "bitcoin", size: "md", class: "text-white" %> + <div class="w-10 h-10 rounded-full bg-brand-coinbase flex items-center justify-center"> + <%= icon "bitcoin", size: "md", class: "text-inverse" %> </div>Note: If
bg-brand-coinbasedoesn't exist insure-design-system.css, add it or use an existing token likebg-accent.As per coding guidelines: "Use Tailwind CSS design tokens (e.g., text-primary, bg-container) instead of raw Tailwind classes" and "Always use functional tokens defined in design system such as
text-primaryinstead oftext-white."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_coinbase_panel.html.erb` around lines 21 - 22, Replace the hardcoded Tailwind color utilities in the Coinbase icon wrapper: change the div class that contains bg-[`#0052FF`] and the icon call that uses text-white to use design-system tokens instead (e.g., replace bg-[`#0052FF`] with a token like bg-brand-coinbase or an existing token such as bg-accent/bg-container, and replace text-white with a functional token like text-inverse or text-primary). If bg-brand-coinbase does not exist in the design system, add it to sure-design-system.css first; update the div with the new bg token and update the icon invocation (icon "bitcoin") to use the chosen text token so the element responds to theming.
76-76: ⚡ Quick winReplace raw Tailwind focus ring color with design-system token.
The focus ring uses
focus:ring-gray-900, which is a raw Tailwind color utility. Use a functional design-system token instead (e.g.,focus:ring-primary).♻️ Replace with design-system token
- class: "inline-flex items-center justify-center rounded-lg px-4 py-2 text-sm font-medium text-inverse bg-inverse hover:bg-inverse-hover focus:outline-none focus:ring-2 focus:ring-gray-900 focus:ring-offset-2 transition-colors" %> + class: "inline-flex items-center justify-center rounded-lg px-4 py-2 text-sm font-medium text-inverse bg-inverse hover:bg-inverse-hover focus:outline-none focus:ring-2 focus:ring-primary focus:ring-offset-2 transition-colors" %>As per coding guidelines: "Always use functional tokens defined in design system such as
text-primaryinstead oftext-white,bg-containerinstead ofbg-white,border border-primaryinstead ofborder border-gray-200."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_coinbase_panel.html.erb` at line 76, The focus ring utility in the button class uses the raw Tailwind color token `focus:ring-gray-900`; update that utility to the design-system functional token (e.g., replace `focus:ring-gray-900` with `focus:ring-primary`) wherever the class string containing `inline-flex items-center ... focus:ring-gray-900` is defined (the class attribute on the Coinbase panel button) so the component uses the design-system token instead of a raw color.app/controllers/settings/providers_controller.rb (3)
108-131: 💤 Low valueSilent redirect on unknown
provider_key.If a request supplies a
provider_keythat's neither inFAMILY_PANELSnor in the configuration registry,connect_formredirects tosettings_providers_pathwith no flash. This will look like a no-op to the user. Consider analert:notice (with a generic "Provider not found" key) for clarity and easier debugging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/settings/providers_controller.rb` around lines 108 - 131, The connect_form action currently redirects to settings_providers_path silently when provider_key is not found in FAMILY_PANELS or Provider::ConfigurationRegistry; change the final redirect to include an alert (e.g., use redirect_to settings_providers_path, alert: t("settings.providers.not_found")) so users see a "Provider not found" message and it's easier to debug unknown provider_key values referenced by connect_form, FAMILY_PANELS, and Provider::ConfigurationRegistry.
286-313: 💤 Low valueNaming collision around "stale".
has_errorflags syncs wheres.stale?is true (a sync that has been running too long), and the returned:stalefield flags last-completed older than 24h ("data freshness"). Two different concepts share the same word, which makes downstream consumers ambiguous. Consider renaming the second to:data_stale(or similar) so callers — and the status pill helper — don't conflate them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/settings/providers_controller.rb` around lines 286 - 313, The returned hash from sync_health_for conflates two different meanings of "stale": the local variable has_error checks s.stale? (a running sync) while the returned key stale represents data freshness; rename the returned key to data_stale (or similar) and update all callers (e.g., compute_provider_sync_health consumers and any status pill helper or view code that reads :stale) to use the new key so the running-sync flag (has_error) and data-freshness flag are unambiguous; ensure you update any tests and serialization that expect :stale to :data_stale and keep the logic for computing stale (last_synced < 24.hours.ago) unchanged.
97-106: 💤 Low value
syncsilently no-ops when there are no syncable items.If
itemsis empty (or all are already syncing), nothing is enqueued but the user still sees the "sync in progress" notice. Consider a distinct notice (or no flash) when nothing was scheduled, so users aren't misled.Suggested change
items = syncable_type.constantize.where(family: Current.family).syncable - items.each { |item| item.sync_later unless item.syncing? } - - redirect_to settings_providers_path, notice: t("settings.providers.sync_provider_in_progress") + enqueued = 0 + items.each do |item| + next if item.syncing? + item.sync_later + enqueued += 1 + end + + notice_key = enqueued.positive? ? "settings.providers.sync_provider_in_progress" : "settings.providers.sync_nothing_to_do" + redirect_to settings_providers_path, notice: t(notice_key)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/settings/providers_controller.rb` around lines 97 - 106, The sync action currently always redirects with the "sync in progress" notice even when no items are scheduled; update the sync method (in providers_controller#sync) to collect items that will actually be enqueued (e.g., scheduled = items.reject(&:syncing?).each { |item| item.sync_later }) or use items.select { |i| !i.syncing? } then call sync_later, and set the flash based on scheduled.any? — if scheduled.any? use t("settings.providers.sync_provider_in_progress"), otherwise set a different notice/alert (or no flash) indicating "no items to sync" so users aren't misled.app/views/settings/providers/_provider_form.html.erb (1)
4-15: 💤 Low valueBoth
step_1_htmltranslation keys exist, but verify if EU panel needs separate step 2 and 3 instructions.The dynamic
step_1_htmlkey correctly resolves tosettings.providers.plaid_eu_panel.step_1_htmlfor the EU provider. However,plaid_eu_panelonly definesstep_1_html—it does not havestep_2orstep_3keys. The code hardcodes steps 2 and 3 to always useplaid_panel.*. If the EU setup process requires different instructions for steps 2 and 3, add the missing keys toplaid_eu_paneland update the code to use the dynamic provider key there as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_provider_form.html.erb` around lines 4 - 15, The setup_steps_data block builds translation keys using provider_key and correctly uses step_1_key for step_1_html, but steps 2 and 3 are hardcoded to "settings.providers.plaid_panel.step_2" and "settings.providers.plaid_panel.step_3", which will miss translations for the EU provider; update the code that builds setup_steps_data (the provider_key, step_1_key, and the array construction in setup_steps_data) to derive step_2 and step_3 keys dynamically from provider_key (e.g. "settings.providers.#{provider_key}_panel.step_2" and step_3) or add the missing step_2/step_3 keys to the plaid_eu_panel translations so the t(...) lookups for step_2 and step_3 succeed.app/views/settings/providers/_search_filters.html.erb (1)
9-9: ⚡ Quick winReplace raw focus ring color with a DS token class.
focus:ring-gray-500bypasses the design-token layer; this should use a functional token ring class for theme consistency.Proposed patch
- class="block w-full border border-secondary rounded-md py-2.5 pl-10 pr-3 bg-container focus:ring-gray-500 sm:text-sm"> + class="block w-full border border-secondary rounded-md py-2.5 pl-10 pr-3 bg-container focus:ring-alpha-black-100 sm:text-sm">As per coding guidelines: “Use Tailwind CSS design tokens … instead of raw Tailwind classes” and “Always prefer using functional tokens defined in sure-design-system.css … rather than raw Tailwind utilities.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/settings/providers/_search_filters.html.erb` at line 9, The element in _search_filters.html.erb uses the raw utility focus:ring-gray-500; replace that raw Tailwind color with the project's design-system focus token class from sure-design-system.css (e.g. swap focus:ring-gray-500 for the functional token class used for focus rings such as focus:ring-ds-focus or the project's equivalent token name) so the input uses the DS theme token instead of a hardcoded color; update the class attribute on the same element that currently contains focus:ring-gray-500.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/settings/providers/_enable_banking_panel.html.erb`:
- Around line 2-8: Replace hardcoded user-facing strings in the
_enable_banking_panel template with i18n lookups: move the step texts, the
"Configuration locked" alert title/body, the submit labels ("Save and
connect"/"Update connection") and the "Connect bank" button text into
config/locales/views/settings/en.yml under
settings.providers.enable_banking_panel (suggested keys: step_1_html, step_2,
step_3, configuration_locked.title, configuration_locked.body, save_and_connect,
update_connection, connect_bank), then update the ERB to call
t("settings.providers.enable_banking_panel.step_1_html") etc. (keep step_1 as
HTML-safe if it contains link markup) and replace the inline strings used in the
submit and connect button with t(...) lookups so all user-facing text uses the
t() helper.
In `@app/views/settings/providers/_lunchflow_panel.html.erb`:
- Around line 2-7: The setup step strings and submit button labels in the Lunch
Flow partial are hardcoded; replace them with i18n lookups and add corresponding
keys to config/locales/en.yml. Update the render call that passes steps to
"settings/providers/setup_steps" to use t("providers.lunch_flow.step_1_html")
(or similar keys) for the first step (ensuring the link_to for "Lunch Flow"
remains but the surrounding copy is localized using _html suffix if it contains
HTML), t("providers.lunch_flow.step_2") and t("providers.lunch_flow.step_3") for
the other steps, and replace the submit button labels referenced on the panel
with t("providers.lunch_flow.connect") / t("providers.lunch_flow.connected")
keys; then add these keys and English values into config/locales/en.yml
mirroring the pattern used by Binance/Mercury locales.
In `@test/models/family/syncer_test.rb`:
- Around line 12-13: The test is counting all Plaid items but Family::Syncer
only schedules `@family.plaid_items.syncable`; update the expectation to use the
same scope to avoid false failures by replacing the current plaid_items_count =
`@family.plaid_items.count` with plaid_items_count =
`@family.plaid_items.syncable.count` so it mirrors the syncable logic used for
binance_items and the Family::Syncer scheduling.
---
Outside diff comments:
In `@app/views/settings/providers/_coinstats_panel.html.erb`:
- Around line 35-36: The submit button currently uses a raw Tailwind color
utility `focus:ring-gray-900` in the `form.submit` call; replace that class with
the design-system focus-ring token class used across the app (e.g. the project’s
focus ring token such as `focus:ring-design-focus` or the equivalent token class
your design system provides) so the `class:` string on the `form.submit` no
longer contains `focus:ring-gray-900` but the appropriate token-based class for
consistent theming and dark-mode support.
In `@app/views/settings/providers/_provider_form.html.erb`:
- Around line 27-67: Replace hardcoded user-facing strings in this partial with
i18n lookups: use t(...) instead of the literal "Configuration can be set via
environment variables or overridden below." (the env_configured message),
replace the placeholder fallback "Optional" used when computing placeholder for
form.text_field (see field.default / field.required logic), and replace the
submit label "Save and connect" passed to form.submit. Ensure you call the Rails
t helper with appropriate keys (e.g. settings.providers.env_hint,
settings.providers.optional_placeholder, settings.providers.save_and_connect)
and update locale files accordingly so the view uses t(...) for these three
strings.
---
Nitpick comments:
In `@app/controllers/settings/providers_controller.rb`:
- Around line 108-131: The connect_form action currently redirects to
settings_providers_path silently when provider_key is not found in FAMILY_PANELS
or Provider::ConfigurationRegistry; change the final redirect to include an
alert (e.g., use redirect_to settings_providers_path, alert:
t("settings.providers.not_found")) so users see a "Provider not found" message
and it's easier to debug unknown provider_key values referenced by connect_form,
FAMILY_PANELS, and Provider::ConfigurationRegistry.
- Around line 286-313: The returned hash from sync_health_for conflates two
different meanings of "stale": the local variable has_error checks s.stale? (a
running sync) while the returned key stale represents data freshness; rename the
returned key to data_stale (or similar) and update all callers (e.g.,
compute_provider_sync_health consumers and any status pill helper or view code
that reads :stale) to use the new key so the running-sync flag (has_error) and
data-freshness flag are unambiguous; ensure you update any tests and
serialization that expect :stale to :data_stale and keep the logic for computing
stale (last_synced < 24.hours.ago) unchanged.
- Around line 97-106: The sync action currently always redirects with the "sync
in progress" notice even when no items are scheduled; update the sync method (in
providers_controller#sync) to collect items that will actually be enqueued
(e.g., scheduled = items.reject(&:syncing?).each { |item| item.sync_later }) or
use items.select { |i| !i.syncing? } then call sync_later, and set the flash
based on scheduled.any? — if scheduled.any? use
t("settings.providers.sync_provider_in_progress"), otherwise set a different
notice/alert (or no flash) indicating "no items to sync" so users aren't misled.
In `@app/views/settings/providers/_coinbase_panel.html.erb`:
- Around line 21-22: Replace the hardcoded Tailwind color utilities in the
Coinbase icon wrapper: change the div class that contains bg-[`#0052FF`] and the
icon call that uses text-white to use design-system tokens instead (e.g.,
replace bg-[`#0052FF`] with a token like bg-brand-coinbase or an existing token
such as bg-accent/bg-container, and replace text-white with a functional token
like text-inverse or text-primary). If bg-brand-coinbase does not exist in the
design system, add it to sure-design-system.css first; update the div with the
new bg token and update the icon invocation (icon "bitcoin") to use the chosen
text token so the element responds to theming.
- Line 76: The focus ring utility in the button class uses the raw Tailwind
color token `focus:ring-gray-900`; update that utility to the design-system
functional token (e.g., replace `focus:ring-gray-900` with `focus:ring-primary`)
wherever the class string containing `inline-flex items-center ...
focus:ring-gray-900` is defined (the class attribute on the Coinbase panel
button) so the component uses the design-system token instead of a raw color.
In `@app/views/settings/providers/_coinstats_panel.html.erb`:
- Around line 4-6: Remove the redundant .html_safe calls on the translation
helpers: replace occurrences of t("coinstats_items.new.step1_html").html_safe
and t("coinstats_items.new.step3_html", accounts_url: accounts_path).html_safe
with plain t("coinstats_items.new.step1_html") and
t("coinstats_items.new.step3_html", accounts_url: accounts_path) respectively,
since Rails marks `_html` I18n keys as HTML-safe automatically.
In `@app/views/settings/providers/_provider_form.html.erb`:
- Around line 4-15: The setup_steps_data block builds translation keys using
provider_key and correctly uses step_1_key for step_1_html, but steps 2 and 3
are hardcoded to "settings.providers.plaid_panel.step_2" and
"settings.providers.plaid_panel.step_3", which will miss translations for the EU
provider; update the code that builds setup_steps_data (the provider_key,
step_1_key, and the array construction in setup_steps_data) to derive step_2 and
step_3 keys dynamically from provider_key (e.g.
"settings.providers.#{provider_key}_panel.step_2" and step_3) or add the missing
step_2/step_3 keys to the plaid_eu_panel translations so the t(...) lookups for
step_2 and step_3 succeed.
In `@app/views/settings/providers/_search_filters.html.erb`:
- Line 9: The element in _search_filters.html.erb uses the raw utility
focus:ring-gray-500; replace that raw Tailwind color with the project's
design-system focus token class from sure-design-system.css (e.g. swap
focus:ring-gray-500 for the functional token class used for focus rings such as
focus:ring-ds-focus or the project's equivalent token name) so the input uses
the DS theme token instead of a hardcoded color; update the class attribute on
the same element that currently contains focus:ring-gray-500.
In `@app/views/settings/providers/_simplefin_panel.html.erb`:
- Around line 2-7: The template renders setup steps with hardcoded English
strings; replace those literals with i18n lookups using the t() helper and
hierarchical keys (e.g., settings.providers.simplefin_panel.step_1_html, step_2,
step_3 and bridge_link_text) and update the call to render
"settings/providers/setup_steps" to pass the translated strings (step_1_html
should use t(..., link: link_to(...)) to keep the HTML link), and add the
corresponding keys and English values to config/locales/views/settings/en.yml so
translations are available.
- Line 26: The focus ring classes in the button class string on the SimpleFin
panel (the long class attribute in _simplefin_panel.html.erb) use raw Tailwind
colors `focus:ring-gray-900` and `theme-dark:focus:ring-white`; replace those
with the design-system focus tokens (for example `focus:ring-primary` or the
appropriate focus token from sure-design-system.css) so the class string uses
functional tokens instead of raw colors; update the class attribute where that
button's classes are defined to remove `focus:ring-gray-900` and
`theme-dark:focus:ring-white` and add the corresponding design-system token(s)
for both light and dark theme focus states.
In `@app/views/settings/providers/_sophtron_panel.html.erb`:
- Around line 43-44: The submit button in the form uses a long inline class
string on the form.submit call; extract that styling into a reusable
design-system button component or helper (e.g., DS::Button or a view helper like
sophtron_button_class) and replace the inline class usage in the form.submit
invocation (the call that uses is_new_record and
t("sophtron_items.sophtron_panel.save"/"update")) so the markup passes a single
component/helper and the label (derived from is_new_record and t(...)) while the
component encapsulates the utility classes (text-inverse, bg-inverse,
bg-inverse-hover, focus:ring-primary, etc.) and ensures consistent styling
across forms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4d72151-fe04-4165-9344-4d6c34e9dd59
📒 Files selected for processing (70)
Procfile.devapp/components/DS/alert.html.erbapp/components/settings/provider_card.html.erbapp/components/settings/provider_card.rbapp/controllers/settings/providers_controller.rbapp/helpers/settings_helper.rbapp/javascript/controllers/providers_filter_controller.jsapp/jobs/sync_all_providers_job.rbapp/models/family/syncer.rbapp/models/provider/metadata.rbapp/models/provider/plaid_adapter.rbapp/models/provider/plaid_eu_adapter.rbapp/views/settings/_section.html.erbapp/views/settings/_settings_nav.html.erbapp/views/settings/providers/_binance_panel.html.erbapp/views/settings/providers/_coinbase_panel.html.erbapp/views/settings/providers/_coinstats_panel.html.erbapp/views/settings/providers/_connection_row.html.erbapp/views/settings/providers/_drawer_header.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/views/settings/providers/_group_heading.html.erbapp/views/settings/providers/_indexa_capital_panel.html.erbapp/views/settings/providers/_lunchflow_panel.html.erbapp/views/settings/providers/_maturity_badge.html.erbapp/views/settings/providers/_mercury_panel.html.erbapp/views/settings/providers/_provider_form.html.erbapp/views/settings/providers/_search_filters.html.erbapp/views/settings/providers/_setup_steps.html.erbapp/views/settings/providers/_simplefin_panel.html.erbapp/views/settings/providers/_snaptrade_panel.html.erbapp/views/settings/providers/_sophtron_panel.html.erbapp/views/settings/providers/_status_pill.html.erbapp/views/settings/providers/_sync_button.html.erbapp/views/settings/providers/connect_form.html.erbapp/views/settings/providers/show.html.erbconfig/locales/views/coinstats_items/ca.ymlconfig/locales/views/coinstats_items/de.ymlconfig/locales/views/coinstats_items/en.ymlconfig/locales/views/coinstats_items/es.ymlconfig/locales/views/coinstats_items/fr.ymlconfig/locales/views/coinstats_items/hu.ymlconfig/locales/views/coinstats_items/nl.ymlconfig/locales/views/coinstats_items/pl.ymlconfig/locales/views/indexa_capital_items/de.ymlconfig/locales/views/indexa_capital_items/en.ymlconfig/locales/views/indexa_capital_items/es.ymlconfig/locales/views/indexa_capital_items/fr.ymlconfig/locales/views/indexa_capital_items/hu.ymlconfig/locales/views/indexa_capital_items/pl.ymlconfig/locales/views/mercury_items/en.ymlconfig/locales/views/mercury_items/hu.ymlconfig/locales/views/settings/de.ymlconfig/locales/views/settings/en.ymlconfig/locales/views/settings/es.ymlconfig/locales/views/settings/fr.ymlconfig/locales/views/settings/hu.ymlconfig/locales/views/settings/pl.ymlconfig/locales/views/settings/pt-BR.ymlconfig/locales/views/snaptrade_items/de.ymlconfig/locales/views/snaptrade_items/en.ymlconfig/locales/views/snaptrade_items/es.ymlconfig/locales/views/snaptrade_items/fr.ymlconfig/locales/views/snaptrade_items/hu.ymlconfig/locales/views/snaptrade_items/pl.ymlconfig/locales/views/sophtron_items/en.ymlconfig/locales/views/sophtron_items/hu.ymltest/controllers/settings/providers_controller_test.rbtest/models/family/syncer_test.rbtest/system/settings/providers_test.rbtest/system/settings_test.rb
💤 Files with no reviewable changes (31)
- config/locales/views/settings/de.yml
- config/locales/views/coinstats_items/hu.yml
- config/locales/views/settings/es.yml
- config/locales/views/settings/pt-BR.yml
- config/locales/views/sophtron_items/en.yml
- config/locales/views/settings/pl.yml
- app/models/provider/plaid_adapter.rb
- config/locales/views/indexa_capital_items/fr.yml
- config/locales/views/snaptrade_items/pl.yml
- config/locales/views/sophtron_items/hu.yml
- config/locales/views/snaptrade_items/de.yml
- config/locales/views/indexa_capital_items/hu.yml
- config/locales/views/snaptrade_items/es.yml
- config/locales/views/indexa_capital_items/en.yml
- config/locales/views/indexa_capital_items/pl.yml
- config/locales/views/mercury_items/hu.yml
- app/models/provider/plaid_eu_adapter.rb
- config/locales/views/coinstats_items/de.yml
- config/locales/views/snaptrade_items/hu.yml
- config/locales/views/settings/hu.yml
- config/locales/views/coinstats_items/ca.yml
- config/locales/views/coinstats_items/pl.yml
- config/locales/views/settings/fr.yml
- config/locales/views/mercury_items/en.yml
- config/locales/views/coinstats_items/fr.yml
- config/locales/views/snaptrade_items/fr.yml
- config/locales/views/coinstats_items/es.yml
- config/locales/views/indexa_capital_items/es.yml
- config/locales/views/indexa_capital_items/de.yml
- config/locales/views/coinstats_items/nl.yml
- config/locales/views/coinstats_items/en.yml
✅ Files skipped from review due to trivial changes (5)
- app/components/DS/alert.html.erb
- Procfile.dev
- app/views/settings/providers/_setup_steps.html.erb
- config/locales/views/snaptrade_items/en.yml
- test/system/settings_test.rb
🚧 Files skipped from review as they are similar to previous changes (8)
- app/components/settings/provider_card.html.erb
- app/jobs/sync_all_providers_job.rb
- app/views/settings/providers/_sync_button.html.erb
- app/views/settings/_settings_nav.html.erb
- app/views/settings/providers/_group_heading.html.erb
- app/views/settings/providers/connect_form.html.erb
- app/views/settings/_section.html.erb
- app/helpers/settings_helper.rb
Fixed:
- Localize the setup steps in _enable_banking_panel,
_lunchflow_panel, and _simplefin_panel. The em-dash sweep had
rewritten these into hardcoded English; they now route through
settings.providers.{enable_banking,lunchflow,simplefin}_panel
step_1_html / step_2 / step_3 keys, mirroring the plaid_panel
treatment.
- connect_form: silent redirect when provider_key is unknown now
carries an alert (settings.providers.not_found) so misrouted
links don't drop users on the page with no feedback.
- sync action: redirect notice now reflects whether anything was
actually scheduled — adds settings.providers.sync_provider_no_items
for the "all items already syncing or none exist" path.
- Family::Syncer test: count plaid_items via the .syncable scope to
match what Family::Syncer actually schedules (already done for
binance_items in the same test).
Skipped, with reasons:
- focus:ring-gray-500/-gray-900 in coinstats / coinbase / simplefin /
search_filters: tracked under issue #1715 as part of the raw-palette
→ DS-token sweep across the whole codebase.
- Coinbase #0052FF brand-color wrapper: tracked under PR #1710's
follow-up tracking comment as the deferred Provider::Metadata
colour-palette decision (designer §01).
- Sophtron submit-button extraction into DS::Button: same
deferred sweep — every panel hand-rolls this class string;
one-off extraction would just churn.
- Redundant .html_safe on _html keys in coinstats: tracked in #1715.
- _provider_form.html.erb env hint, "Optional" placeholder, "Save and
connect" submit: pre-existing strings not added on this branch.
- Renaming sync_health_for's :stale to :data_stale: pre-existing
shape, refactor scope.
- Plaid_eu using plaid_panel.step_2/step_3 keys: deliberate. Same
English copy across both providers; duplicating keys would just
give translators twice the work for identical strings.
- _enable_banking_panel / _lunchflow_panel / _simplefin_panel
alert + submit + button labels: pre-existing hardcoded strings
from before this branch. Setup steps were the strings actually
touched in the em-dash sweep, so those got localized; the rest
belong in a broader panel-i18n pass.
Verified:
- bundle exec erb_lint on the three panels: clean.
- bin/rubocop on controller + test: clean.
- bin/rails test test/models/family/syncer_test.rb
test/controllers/settings/providers_controller_test.rb:
23 runs, 85 assertions, 0 failures.
- DISABLE_PARALLELIZATION=true bin/rails test
test/system/settings/providers_test.rb:
15 runs, 38 assertions, 0 failures.
|
@coderabbitai thanks for the pass — went through each finding against the current branch and validated. Landed in Fixed
Skipped, with reasons
Validated
|
|
Tip For best results, initiate chat on the files or code changes. The PR description image shows the Bank Sync settings page (
All test suites green (unit + system) with the fixes in place — good to go from my side. ( •ᴗ• )🐇✨ |
Signed-off-by: Guillem Arias Fauste <accounts@gariasf.com>
Main's PR #1705 (Sophtron manual sync) shipped a migration with the same 20260508120000 timestamp as our add_last_sync_all_attempted_at_to_families migration. The merge that brought main into this branch left both files at the same prefix, which trips Rails' "Duplicate migration" guard at db:schema:load time and broke CI. Renaming our migration to 20260510120000 keeps the column it adds intact (already in db/schema.rb) and bumps the schema version to match. No DB-level change.











Summary
Completes the "Option A" Claude Design redesign of the Bank Sync settings page.
DS::Dialogdrawer;Provider::Metadataregistry for all 11 providers; maturity pills also surface on connected accordion rows:off(Available) instead of:warn(Your connections)/settings/bank_sync: 301 permanent redirect; nav collapsed from two entries to a single "Bank Sync"; page title and lede refreshedScreenshot
Test plan
/settings/bank_sync→ 301 redirect to/settings/providersbin/rails testgreenDISABLE_PARALLELIZATION=true bin/rails test:systemgreen🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactor